[WIP] CONSOLE-5091: Add bulk selection and schedulable actions to Nodes page#16203
[WIP] CONSOLE-5091: Add bulk selection and schedulable actions to Nodes page#16203rhamilto wants to merge 10 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamilto: This pull request references CONSOLE-5091 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rhamilto
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
component/helm
|
@rhamilto: This pull request references CONSOLE-5091 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@rhamilto: This pull request references CONSOLE-5091 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request introduces comprehensive enhancements to OpenShift Console's data view and resource fetching patterns. Changes include new bulk selection and bulk action capabilities for ConsoleDataView with supporting hooks (useDataViewSelection, ConsoleDataViewBulkSelect), resizable column support across multiple resource list components, and new bulk node actions for scheduling operations. A significant refactoring replaces Firehose-based resource fetching with useK8sWatchResource/useK8sWatchResources hooks across container, RBAC binding, cluster-settings, monitoring, and template components. Infrastructure improvements add deprecated shared module warnings to the webpack plugin, null-safety refinements throughout bindings and component navigation, and Cypress test updates to skip operator-related integration tests. Locale additions support node scheduling prompts and service account selection strings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.ts (1)
21-34: Consider extracting shared cleanup helper to reduce duplication.This
cleanupOperatorResourcesimplementation is nearly identical to the one inoperator-install-single-namespace.cy.ts. The only difference is namespace handling (hardcodedGlobalInstalledNamespacevs. parameterizednamespace).When these tests are re-enabled, consider extracting this to
operator.view.tsor a shared test utilities module to avoid maintaining duplicate code:// In operator.view.ts or a shared helper export const cleanupOperatorResources = (packageName: string, namespace: string) => { ... }Not blocking since the tests are skipped, but worth addressing when re-enabling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.ts` around lines 21 - 34, The cleanupOperatorResources function is duplicated between this file and operator-install-single-namespace.cy.ts; refactor by extracting a shared helper (e.g., export const cleanupOperatorResources(packageName: string, namespace: string)) into a common module such as operator.view.ts or a test utilities file, then replace the local cleanupOperatorResources with calls to that shared function using operatorPackageName and GlobalInstalledNamespace here (and the parameterized namespace in the other test) to remove duplication and centralize the oc delete logic.frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.ts (1)
25-25: Consider adding a tracking comment for skipped tests.Skipping the entire uninstall test suite via
xdescriberemoves coverage for critical operator uninstall error scenarios (operand load failures, deletion failures, successful cleanup). While I understand flaky tests need to be disabled to unblock CI, it's good practice to add a// TODO(JIRA-XXXX): Re-enable when flakiness is resolvedcomment so these don't become permanently forgotten.Without tracking, these tend to rot indefinitely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.ts` at line 25, The test suite is being fully skipped with xdescribe(`Testing uninstall of ${testOperator.name} Operator`), which can cause the uninstall tests to be forgotten; add a clear tracking comment directly above the xdescribe line such as // TODO(JIRA-XXXX): Re-enable uninstall tests for ${testOperator.name} when flakiness is fixed — include brief reason (flaky operand/delete flows), owner or team and a link to the ticket; optionally convert xdescribe to describe.skip to make the skip intent explicit while preserving the same behavior.frontend/packages/console-app/src/components/nodes/NodeTerminal.tsx (1)
155-155: Minor: Optional chaining is now redundant after the guard.Since we return early on line 151-153 when
!pod, the optional chaining onpod?.status?.phaseis technically unnecessary. Not a blocker—just a tiny cleanup opportunity if you're in this area again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/NodeTerminal.tsx` at line 155, The switch uses redundant optional chaining after an earlier guard that returns when pod is falsy; update the switch expression from using pod?.status?.phase to pod.status.phase (or otherwise drop the leading ? on status) in NodeTerminal's render logic so it relies on the existing early return for pod, keeping the rest of the switch cases the same.frontend/public/components/storage-class-form.tsx (1)
736-738: Tighten theresourcescontract and pass the hook result through directly.This form only consumes
scandcsilist results, but the new prop type allows arbitrary keys and singular-or-list payloads, which keeps the list assumption hidden behind the casts at Lines 254-255. Lines 793-804 then rebuild fresh result objects every render. Typing those two keys explicitly and passingwatchedResourcesthrough directly will make the refactor easier to maintain and reduce extra prop churn in the connected child. As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Also applies to: 791-804
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/storage-class-form.tsx` around lines 736 - 738, The resources prop should be tightened to explicitly declare the two keys used ("sc" and "csi") with their exact payload shapes instead of a loose index signature and union types; update the prop/type where resources?: { [key: string]: WatchK8sResultsObject<...> } to resources?: { sc: WatchK8sResultsObject<K8sResourceCommon[]>; csi: WatchK8sResultsObject<K8sResourceCommon[]> } (or singular types if one is singular) so callers stop casting at the usage site in the component (see the code that casts at the former Lines 254-255) and then stop rebuilding result objects on each render (remove the reconstruction in the block around Lines 793-804) by passing the hook return (watchedResources) through directly to the child component that expects {sc, csi}; ensure the prop name and types match the hook that produces watchedResources to keep types consistent.frontend/public/components/instantiate-template.tsx (1)
406-415: Add type parameter for type safety.
useK8sWatchResourceis called without a type parameter, sotemplateis typed asunknown. This could lead to type errors downstream or require unsafe casts. Consider adding the generic type.🔧 Add type parameter
- const [template, loaded, loadError] = useK8sWatchResource( + const [template, loaded, loadError] = useK8sWatchResource<TemplateKind>( templateName && templateNamespace🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/instantiate-template.tsx` around lines 406 - 415, The call to useK8sWatchResource lacks a generic type so template is inferred as unknown; update the call site to provide the appropriate resource type (e.g., the Template/TemplateKind interface you use in the codebase) as the generic parameter to useK8sWatchResource so that the returned template variable has the correct typed shape; modify the expression where useK8sWatchResource is invoked (referencing useK8sWatchResource and the template variable) to include the type parameter matching your Template type.frontend/public/components/factory/details.tsx (2)
110-114: Verify objData shape when props.obj is nil.When
props.objis nil,watchedResources.objis used. This returns{ data, loaded, loadError }, but there's no explicit handling if the watch fails or returns unexpected data before it's passed downstream. Ensure consumers handle potential undefined/nulldatagracefully.🔍 Safer objData derivation
- const objData = _.isNil(props.obj) ? watchedResources.obj : props.obj; + const objData = _.isNil(props.obj) + ? watchedResources.obj ?? { data: undefined, loaded: false, loadError: undefined } + : props.obj;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/factory/details.tsx` around lines 110 - 114, The derivation of objData blindly uses watchedResources.obj when props.obj is nil but does not validate the shape (data/loaded/loadError); update the logic around useK8sWatchResources and the objData assignment so you explicitly check watchedResources.obj and its properties (e.g., watchedResources.obj?.data, watchedResources.obj?.loaded, watchedResources.obj?.loadError), default to a safe value (null or an empty object) when data is undefined or a loadError exists, and ensure downstream consumers receive that safe value rather than an unexpected shape; locate this in the code that sets watchedResources and the objData constant to add these guards.
127-149: Spreading watchedResources may pass unintended props.Spreading
{...watchedResources}intoConnectedPageHeadingpasses all watched resources as props. If new resources are added to the watch config, they'll automatically be passed to the heading component. This coupling could lead to unexpected behavior. Consider being explicit about which resources are passed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/factory/details.tsx` around lines 127 - 149, Spreading {...watchedResources} into ConnectedPageHeading risks leaking all watched keys into the heading; instead explicitly pass only the required fields (e.g., pass watchedResources.items or individual props like watchedResources.foo, watchedResources.bar) to ConnectedPageHeading so future additions to watchedResources won’t become unintended props. Locate the ConnectedPageHeading usage in details.tsx and replace the spread of watchedResources with explicit prop names (or wrap watchedResources into a single explicit prop name such as watchedResources={watchedResources} or watchedResourceList={watchedResources.items}) ensuring consumers (ConnectedPageHeading) receive only the intended data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/integration-tests-cypress/views/details-page.ts`:
- Around line 16-19: The isLoaded() readiness check is too weak; after ensuring
the skeleton ('skeleton-detail-view') is gone, update the assertion on
'[data-test-id="resource-title"]' to first assert visibility
(should('be.visible')) and then assert trimmed, non-whitespace content by
invoking the element text and checking it is not empty (e.g., invoke('text') and
assert text.trim() contains non-whitespace or matches /\S/); make this change
inside the isLoaded function to replace the current .should('not.be.empty')
check.
In `@frontend/public/components/command-line-tools.tsx`:
- Around line 106-117: The component currently only shows LoadingBox when
!loaded and !loadError, but when loadError is truthy it still falls through to
render CommandLineTools; update the conditional to explicitly handle the error
case by returning an error/status UI (e.g., render StatusBox or a dedicated
error message) when loadError is set, ensure you pass the same props or error
details to CommandLineTools if you still want it mounted, and add any required
import for StatusBox; refer to the symbols loaded, loadError, CommandLineTools,
LoadingBox, and StatusBox to locate where to add the new branch.
In `@frontend/public/components/container.tsx`:
- Around line 513-519: ConnectedPageHeading is being passed loadError but
HorizontalNav is not; update the HorizontalNav invocation to include the same
loadError so its StatusBox can render errors consistently. Specifically, in the
component where ConnectedPageHeading and HorizontalNav are rendered, add
loadError: props.loadError to the obj prop passed to HorizontalNav (the
HorizontalNav call that currently passes obj={{ data: props.pod, loaded:
props.loaded }}), ensuring ContainerDetailsList, props.pod, props.loaded and
props.loadError are all forwarded consistently.
In `@frontend/public/components/create-yaml.tsx`:
- Around line 128-134: The current error handling in the create-yaml component
returns ErrorPage404 for any loadError, which misrepresents errors like 403 or
network failures; update the render logic around the loaded and loadError checks
(variables loaded, loadError) to inspect loadError (e.g., status or
error.type/message) and render an appropriate component instead of always
ErrorPage404—either map specific statuses to dedicated components (e.g.,
ForbiddenPage for 403) or introduce a GenericError component that displays the
actual error.message/status so users see the real failure instead of a 404.
In `@frontend/public/components/factory/list-page.tsx`:
- Around line 533-565: The watchResources builder omits resource-level filters
so ListPageProps.filters are not applied to useK8sWatchResources; update the
reduce that creates watchResources (based on k8sResources) to copy r.filters
into each watch config (e.g., include filters: r.filters) so the produced record
passed to useK8sWatchResources preserves resources[].filters and downstream list
rendering and row counts are filtered correctly; modify the reduce in the
watchResources useMemo that constructs acc[key] to include the filters property
from r.
- Around line 176-177: The bug is that FireMan is being given the raw action
creator filterList (propsFilterList) but applyFilterInternal calls it and
ignores the returned action, so dispatch never occurs; update the place where
FireMan is constructed (and any other occurrences around lines with reduxIDs/
filterList and where applyFilterInternal is used) to pass a bound dispatcher
function instead of the raw creator—i.e., wrap propsFilterList with dispatch (or
call props.dispatch(propsFilterList(...))) so applyFilter / applyFilterInternal
receive and return a dispatched action; ensure functions named FireMan,
applyFilterInternal, filterList (propsFilterList) and applyFilter are updated
accordingly so URL-applied filters and applyFilter consumers receive the
dispatched result.
- Around line 108-111: The current selection of resourceData uses
watchedResources ?? resources which picks an empty object from
useK8sWatchResources on first render and breaks callers; update the logic in the
list-page component to only use watchedResources when it actually has entries
(e.g. check watchedResources is truthy and Object.keys(watchedResources).length
> 0) otherwise fall back to resources, and keep the subsequent use of
flatten(resourceData) unchanged (references: watchedResources, resources,
resourceData, flatten, useK8sWatchResources).
---
Nitpick comments:
In `@frontend/packages/console-app/src/components/nodes/NodeTerminal.tsx`:
- Line 155: The switch uses redundant optional chaining after an earlier guard
that returns when pod is falsy; update the switch expression from using
pod?.status?.phase to pod.status.phase (or otherwise drop the leading ? on
status) in NodeTerminal's render logic so it relies on the existing early return
for pod, keeping the rest of the switch cases the same.
In
`@frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.ts`:
- Around line 21-34: The cleanupOperatorResources function is duplicated between
this file and operator-install-single-namespace.cy.ts; refactor by extracting a
shared helper (e.g., export const cleanupOperatorResources(packageName: string,
namespace: string)) into a common module such as operator.view.ts or a test
utilities file, then replace the local cleanupOperatorResources with calls to
that shared function using operatorPackageName and GlobalInstalledNamespace here
(and the parameterized namespace in the other test) to remove duplication and
centralize the oc delete logic.
In
`@frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.ts`:
- Line 25: The test suite is being fully skipped with xdescribe(`Testing
uninstall of ${testOperator.name} Operator`), which can cause the uninstall
tests to be forgotten; add a clear tracking comment directly above the xdescribe
line such as // TODO(JIRA-XXXX): Re-enable uninstall tests for
${testOperator.name} when flakiness is fixed — include brief reason (flaky
operand/delete flows), owner or team and a link to the ticket; optionally
convert xdescribe to describe.skip to make the skip intent explicit while
preserving the same behavior.
In `@frontend/public/components/factory/details.tsx`:
- Around line 110-114: The derivation of objData blindly uses
watchedResources.obj when props.obj is nil but does not validate the shape
(data/loaded/loadError); update the logic around useK8sWatchResources and the
objData assignment so you explicitly check watchedResources.obj and its
properties (e.g., watchedResources.obj?.data, watchedResources.obj?.loaded,
watchedResources.obj?.loadError), default to a safe value (null or an empty
object) when data is undefined or a loadError exists, and ensure downstream
consumers receive that safe value rather than an unexpected shape; locate this
in the code that sets watchedResources and the objData constant to add these
guards.
- Around line 127-149: Spreading {...watchedResources} into ConnectedPageHeading
risks leaking all watched keys into the heading; instead explicitly pass only
the required fields (e.g., pass watchedResources.items or individual props like
watchedResources.foo, watchedResources.bar) to ConnectedPageHeading so future
additions to watchedResources won’t become unintended props. Locate the
ConnectedPageHeading usage in details.tsx and replace the spread of
watchedResources with explicit prop names (or wrap watchedResources into a
single explicit prop name such as watchedResources={watchedResources} or
watchedResourceList={watchedResources.items}) ensuring consumers
(ConnectedPageHeading) receive only the intended data.
In `@frontend/public/components/instantiate-template.tsx`:
- Around line 406-415: The call to useK8sWatchResource lacks a generic type so
template is inferred as unknown; update the call site to provide the appropriate
resource type (e.g., the Template/TemplateKind interface you use in the
codebase) as the generic parameter to useK8sWatchResource so that the returned
template variable has the correct typed shape; modify the expression where
useK8sWatchResource is invoked (referencing useK8sWatchResource and the template
variable) to include the type parameter matching your Template type.
In `@frontend/public/components/storage-class-form.tsx`:
- Around line 736-738: The resources prop should be tightened to explicitly
declare the two keys used ("sc" and "csi") with their exact payload shapes
instead of a loose index signature and union types; update the prop/type where
resources?: { [key: string]: WatchK8sResultsObject<...> } to resources?: { sc:
WatchK8sResultsObject<K8sResourceCommon[]>; csi:
WatchK8sResultsObject<K8sResourceCommon[]> } (or singular types if one is
singular) so callers stop casting at the usage site in the component (see the
code that casts at the former Lines 254-255) and then stop rebuilding result
objects on each render (remove the reconstruction in the block around Lines
793-804) by passing the hook return (watchedResources) through directly to the
child component that expects {sc, csi}; ensure the prop name and types match the
hook that produces watchedResources to keep types consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2c3fbeee-cf14-45d1-b293-e64867a50e9e
📒 Files selected for processing (53)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/actions/hooks/useBindingActions.tsfrontend/packages/console-app/src/components/data-view/ConsoleDataView.tsxfrontend/packages/console-app/src/components/data-view/ConsoleDataViewBulkSelect.tsxfrontend/packages/console-app/src/components/data-view/dataViewSelectionHelpers.tsfrontend/packages/console-app/src/components/data-view/useDataViewSelection.tsfrontend/packages/console-app/src/components/nodes/NodeTerminal.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/nodes/useBulkNodeActions.tsxfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.mdfrontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.tsfrontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.tsfrontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.tsfrontend/packages/helm-plugin/src/components/details-page/resources/__tests__/HelmReleaseResources.spec.tsxfrontend/packages/integration-tests-cypress/views/details-page.tsfrontend/packages/operator-lifecycle-manager-v1/locales/en/olm-v1.jsonfrontend/packages/operator-lifecycle-manager-v1/src/components/cluster-extension/ServiceAccountDropdown.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-single-namespace.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.tsfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsxfrontend/public/components/RBAC/bindings.tsxfrontend/public/components/__tests__/container.spec.tsxfrontend/public/components/cluster-settings/cluster-settings.tsxfrontend/public/components/command-line-tools.tsxfrontend/public/components/console-notifier.tsxfrontend/public/components/container.tsxfrontend/public/components/control-plane-machine-set.tsxfrontend/public/components/create-yaml.tsxfrontend/public/components/cron-job.tsxfrontend/public/components/edit-yaml.tsxfrontend/public/components/factory/__tests__/details.spec.tsxfrontend/public/components/factory/__tests__/list-page.spec.tsxfrontend/public/components/factory/details.tsxfrontend/public/components/factory/list-page.tsxfrontend/public/components/instantiate-template.tsxfrontend/public/components/machine-autoscaler.tsxfrontend/public/components/machine-config-pool.tsxfrontend/public/components/machine-config.tsxfrontend/public/components/machine-health-check.tsxfrontend/public/components/machine-set.tsxfrontend/public/components/machine.tsxfrontend/public/components/monitoring/alertmanager/alertmanager-config.tsxfrontend/public/components/monitoring/alertmanager/alertmanager-yaml-editor.tsxfrontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsxfrontend/public/components/namespace-bar.tsxfrontend/public/components/storage-class-form.tsxfrontend/public/components/utils/horizontal-nav.tsxfrontend/public/components/utils/inject.tsfrontend/public/components/utils/list-dropdown.tsxfrontend/public/locales/en/public.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/operator-lifecycle-manager-v1/src/components/cluster-extension/ServiceAccountDropdown.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.tsfrontend/packages/integration-tests-cypress/views/details-page.tsfrontend/packages/operator-lifecycle-manager-v1/locales/en/olm-v1.jsonfrontend/packages/console-app/src/components/nodes/NodeTerminal.tsxfrontend/public/components/utils/inject.tsfrontend/public/components/control-plane-machine-set.tsxfrontend/packages/helm-plugin/src/components/details-page/resources/__tests__/HelmReleaseResources.spec.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-single-namespace.cy.tsfrontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.tsfrontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.tsfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsxfrontend/public/components/factory/__tests__/list-page.spec.tsxfrontend/public/components/monitoring/alertmanager/alertmanager-yaml-editor.tsxfrontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsxfrontend/public/components/machine-autoscaler.tsxfrontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.tsfrontend/public/components/create-yaml.tsxfrontend/packages/console-app/src/components/data-view/ConsoleDataViewBulkSelect.tsxfrontend/public/components/__tests__/container.spec.tsxfrontend/public/components/machine.tsxfrontend/public/components/monitoring/alertmanager/alertmanager-config.tsxfrontend/public/components/machine-set.tsxfrontend/public/components/machine-health-check.tsxfrontend/public/components/container.tsxfrontend/public/components/machine-config-pool.tsxfrontend/public/components/machine-config.tsxfrontend/packages/console-app/src/components/data-view/useDataViewSelection.tsfrontend/public/components/cluster-settings/cluster-settings.tsxfrontend/packages/console-app/src/components/nodes/useBulkNodeActions.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.tsfrontend/packages/console-app/src/actions/hooks/useBindingActions.tsfrontend/public/components/instantiate-template.tsxfrontend/public/components/namespace-bar.tsxfrontend/public/components/utils/list-dropdown.tsxfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.mdfrontend/public/locales/en/public.jsonfrontend/public/components/console-notifier.tsxfrontend/public/components/utils/horizontal-nav.tsxfrontend/public/components/factory/__tests__/details.spec.tsxfrontend/public/components/command-line-tools.tsxfrontend/packages/console-app/src/components/data-view/dataViewSelectionHelpers.tsfrontend/public/components/factory/details.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/public/components/edit-yaml.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.tsfrontend/public/components/storage-class-form.tsxfrontend/public/components/cron-job.tsxfrontend/packages/console-app/src/components/data-view/ConsoleDataView.tsxfrontend/public/components/factory/list-page.tsxfrontend/packages/console-app/locales/en/console-app.jsonfrontend/public/components/RBAC/bindings.tsx
🪛 LanguageTool
frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md
[grammar] ~18-~18: Use a hyphen to join words.
Context: ...warnings for usage of deprecated Console provided shared modules ([CONSOLE-5135],...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (62)
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.ts (1)
94-94: Nice reliability improvement in test synchronization.Switching from a fixed wait to a timed row lookup makes this flow less flaky and faster under normal conditions.
frontend/public/components/utils/inject.ts (1)
12-15: Good defensive change—child props now take precedence.This is a sensible fix: explicit props on children should not be clobbered by injected props. The logic using
_.omit(safeProps, Object.keys(c.props))is clean and the inline comment clearly documents the intent.Worth noting this is a semantic behavioral change (previously injected props would override). If any existing consumers relied on the old override behavior, they'd need adjustment—but the new behavior is more predictable and aligns with typical React composition patterns.
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.ts (1)
177-177: Selective test skipping is reasonable; consider documenting the reason.Good approach to only skip the tests that require actual operator installation (
create(testDeprecatedSubscription)and subsequent approval workflow) while keeping the mock-based tests enabled. This preserves partial coverage for the deprecation warning UI.However, these three tests cover the "installed operator" deprecation badge workflow which is user-facing. A brief comment explaining the flakiness root cause (e.g., timing issues with InstallPlan approval) would help future maintainers know when it's safe to re-enable.
Also applies to: 206-206, 229-229
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-single-namespace.cy.ts (2)
21-36: Cleanup helper looks solid; good defensive pattern.The
cleanupOperatorResourceshelper using OLM label selectors (operators.coreos.com/${operatorPackageName}.${namespace}) is the correct approach for cleaning up operator-related resources. Using--ignore-not-foundandfailOnNonZeroExit: falseensures the cleanup is idempotent and won't fail the test setup if resources don't exist.The 2-minute timeout per deletion command is generous but appropriate for resource cleanup that may need to wait for finalizers.
38-42: Consider cleanup ordering inbefore()hook.Currently the sequence is:
cy.createProjectWithCLI(testName)(line 41)cleanupOperatorResources(testName)(line 42)This works for cleaning up stale resources from previous failed runs, but since the suite is skipped (
xdescribe), this is moot for now. When re-enabled, verify this order handles the case where a previous run left resources in a partially-created state.frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.ts (1)
47-49: Good addition ofafter()cleanup hook.Adding the
after()hook ensures cleanup runs even if tests fail partway through. This is a reliability improvement over the previous state where cleanup may have only occurred viaoperator.uninstall()within the test itself.frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md (1)
18-18: Changelog entry and references are consistent.The new item and added references are aligned and traceable.
Also applies to: 113-113, 146-147
frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts (2)
124-139: Deprecation warning collection logic looks solid.This is scoped well and only emits warnings for deprecated shared modules that are actually present in plugin dependencies.
476-479: Good choice to surface deprecations as compilation warnings.This keeps builds non-blocking while still making deprecated shared-module usage visible.
frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts (1)
20-21: Metadata extension is clean and backward-compatible.Adding
deprecatedwith explicit defaults and targeted module annotations is a maintainable way to drive the new warning path.Also applies to: 56-57, 68-74
frontend/packages/console-app/locales/en/console-app.json (1)
353-356: LGTM! Well-structured i18n strings for bulk actions.The interpolation variables (
{{count}},{{failureCount}},{{totalCount}}) are correctly applied and follow i18next conventions. Good job providing granular failure feedback messages.frontend/packages/console-app/src/components/nodes/NodeTerminal.tsx (1)
147-153: LGTM! Improved separation of loading vs. missing pod states.The refactored logic correctly distinguishes between "still loading" and "loaded but pod not found", providing clearer feedback when a debug pod gets deleted externally. Good defensive handling.
frontend/public/components/machine-health-check.tsx (2)
70-119: LGTM! Clean implementation of resizable columns.The hook correctly returns both
columnsandresetAllColumnWidths, appliesresizablePropsto data columns while preserving the actions column without resize capability. Memoization dependencies are correct.
128-143: LGTM! ConsoleDataView integration is correct.The destructuring and prop passing for
isResizableandresetAllColumnWidthsfollows the established pattern across this PR.frontend/public/components/machine-config.tsx (2)
196-263: LGTM! Consistent resizable column implementation.The pattern matches the other machine-related list components. Column configuration, memoization, and hook return shape are all correct.
265-284: LGTM! List component integration is correct.frontend/public/components/machine-autoscaler.tsx (2)
104-172: LGTM! Resizable columns correctly configured.Consistent implementation across name, namespace, scale target, min, and max columns. Actions column appropriately excluded from resizing.
174-198: LGTM! ConsoleDataView integration follows the established pattern.frontend/public/components/machine.tsx (2)
201-285: LGTM! Comprehensive resizable column support for all data columns.All seven data columns receive
resizablePropswhile the actions column correctly usesactionsCellPropswithout resize. Memoization is properly configured.
287-306: LGTM! List component correctly wired to ConsoleDataView.frontend/public/components/control-plane-machine-set.tsx (2)
206-274: LGTM! Resizable columns correctly implemented for ControlPlaneMachineSet.Hook return type, column configuration, and memoization all follow the established pattern.
336-360: LGTM! ConsoleDataView integration is correct.frontend/public/components/machine-config-pool.tsx (2)
307-364: LGTM! Resizable columns correctly implemented for MachineConfigPool.The pattern is consistent with the other machine-related list components in this PR.
409-436: LGTM! ConsoleDataView integration follows the established pattern.The
MachineConfigPoolsArePausedAlertpreceding the data view is a nice touch for UX.frontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.ts (1)
361-373: LGTM! Clean API extension for selection support.The type definitions are well-structured with clear JSDoc comments. Using
Set<string>forselectedItemsprovides O(1) membership checks—good choice for potentially large node lists. The optionalonSelectAllkeeps the API flexible for consumers that don't need bulk selection.frontend/packages/console-app/src/components/data-view/ConsoleDataViewBulkSelect.tsx (1)
11-33: LGTM! Clean wrapper with proper PatternFly integration.The component correctly maps PatternFly's
BulkSelectValuevariants to the appropriate callbacks. The treatment ofpageandnonePagevariants as synonyms forall/noneis sensible given the current usage context where all filtered items are visible.frontend/packages/console-app/src/components/data-view/useDataViewSelection.ts (1)
39-89: LGTM! Well-designed selection hook with proper immutability.Good use of functional
setStateto avoid race conditions on rapid clicks. TheselectedItemsderivation correctly filters against currentdata, ensuring orphaned IDs (from deleted items) don't leak into the result array. ThefilterSelectablepredicate is a nice touch for excluding non-selectable rows like CSRs.frontend/packages/console-app/src/components/data-view/dataViewSelectionHelpers.ts (1)
59-78: LGTM! Helpers follow PatternFly's select cell API correctly.The
createSelectionCellfunction properly wires the checkbox selection. Note thatdisable(notdisabled) is intentional—it matches PatternFly's@patternfly/react-tableselect configuration interface.frontend/packages/console-app/src/components/nodes/useBulkNodeActions.tsx (1)
31-81: Good use ofPromise.allSettledfor partial-failure feedback.The implementation correctly reports how many nodes failed and only calls
onCompleteon full success. The localized error messages provide actionable feedback.One consideration: on partial failure, the selection remains intact (since
onCompleteisn't called). This means users can retry the action on the remaining nodes, which is sensible UX. If that's intentional, it's worth a brief comment for future maintainers.frontend/packages/console-app/src/components/nodes/NodesPage.tsx (2)
644-660: LGTM! Selection integration is well-structured.The
filterSelectablepredicate correctly excludes CSR resources from selection, making theNodeKind[]cast on line 658 type-safe. TheonComplete: clearSelectionpattern ensures UI consistency after successful bulk operations.
662-678: Good column layout hygiene—excluding internal columns from management.Filtering out the
selectand actions columns fromcolumnLayout.columnsprevents them from appearing in the column management modal. This is the correct approach for internal/structural columns.frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (2)
105-120: Well-designed default bulk select with filter-awareness.The
defaultBulkSelectcorrectly derives counts fromfilteredData, ensuring "select all" respects active filters. The early return whenbulkSelectis explicitly provided allows consumers to override the default behavior when needed.
272-309: LGTM! Sticky column offsets properly account for selection column.The
SELECTION_COLUMN_WIDTH = '45px'constant is used asstickyLeftOffsetfor the name column, ensuring proper horizontal scroll behavior when both selection and name columns are sticky. The separation of*ColumnPropsvs*CellPropsprovides good composition flexibility.frontend/public/components/machine-set.tsx (2)
280-365: Good alignment with the resizableConsoleDataViewcontract.Returning
columnstogether withresetAllColumnWidths, and splitting the sticky header props tonameCellProps/actionsCellProps, keeps this list consistent with the updated table API.
433-435: Reset column widths affordance is correctly independent of column management UI.ConsoleDataView renders the reset button based on
isResizable && resetAllColumnWidths, nothideColumnManagement. Line 435 properly passesresetAllColumnWidthsto the component, so users can reset widths even with column management hidden. No recovery path issue here.> Likely an incorrect or invalid review comment.frontend/public/components/storage-class-form.tsx (1)
27-29: Hook-based watch migration looks solid.Keeping the watch setup in the wrapper and leaving
StorageClassFormInner's consumption path unchanged keeps the blast radius low.Also applies to: 785-788
frontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsx (1)
566-579: Clean migration of loading state aggregation.The
combinedLoadedandcombinedLoadErrorlogic properly gates rendering until both the Secret watch and the Alertmanager globals fetch complete. Using&&for loaded and||for errors is the correct pattern for dependent data sources.frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx (2)
1275-1278: Loading aggregation logic is correct.The pattern
resourceValues.every((r) => r.loaded || r.loadError)ensures we wait for all resources to either successfully load or fail before proceeding. This is consistent with the loading semantics used elsewhere in this PR.
207-215: No null-safety issue — the code already handles undefined gracefully.The
installModesForfunction uses optional chaining (pkg?.status?.channels?...) with a fallback to[], so passingundefinedreturns an empty array rather than crashing. WhensupportedInstallModesForreduces over an empty array, it returns the initial value (InstallModeType.InstallModeTypeOwnNamespace). Additionally,StatusBoxgates rendering ofOperatorHubSubscribeFormuntil data is loaded, so line 209 won't execute without validpackageManifest.data[0].The proposed optional chaining fix is unnecessary.
> Likely an incorrect or invalid review comment.frontend/public/components/namespace-bar.tsx (1)
130-137: Good use of conditional watch resource.Passing
nullwhenhideProjectsis true correctly disables the Kubernetes watch, avoiding unnecessary API calls. The conditional structure is clean and follows established patterns.frontend/public/components/utils/list-dropdown.tsx (2)
205-244: Solid Firehose-to-hooks migration.The watch resource mapping correctly preserves the original Firehose resource config semantics (using
prop || kindas key). The loaded/error aggregation matches the established pattern.
269-272: Good fix for controlled component state sync.The
useEffectto synchronize internalselectedKeywithprops.selectedKeyensuresNsDropdownbehaves correctly as a controlled component when the parent updates the selected value externally.frontend/public/components/edit-yaml.tsx (2)
163-180: Clean conditional watch based on feature flag.The
useMemocorrectly gates the watch resource config onhasYAMLSampleFlag, preventing unnecessary API calls when the feature is disabled. Good pattern.
100-100: Thecreateprop requirement change is safe—all existing usages already provide it explicitly, either directly or through wrapper components likeDroppableEditYAML(which defaults tofalse). No callers rely on undefined behavior, so this type tightening poses no practical breaking change to the codebase.frontend/public/components/console-notifier.tsx (1)
28-47: Good defensive handling for notification loading failures.Returning
nullon load errors for notifications is appropriate—banner notifications are non-critical UI and shouldn't break the page. Theconsole.errorprovides debugging visibility without disrupting UX.frontend/public/components/RBAC/bindings.tsx (3)
363-380: Well-structured loading and error aggregation.The defensive check
Object.values(resources).some((r) => r.data)before flattening prevents iteration over undefined resources. The loaded/error aggregation follows the established PR pattern consistently.
799-826: Clean separation of loading states in BindingLoadingWrapper.The explicit render branches for loading, error, and empty object states provide clear control flow. The optional chaining at line 816 when building
fixedprevents crashes during the loading transition.
187-191: Good defensive coding for binding type detection.Adding optional chaining for
binding.roleRef?.nameandbinding.metadata?.namespaceprevents potential crashes when processing partially loaded or malformed bindings.frontend/packages/console-app/src/actions/hooks/useBindingActions.ts (1)
36-49: LGTM on null-safety improvements.The defensive approach here is solid—guarding
useK8sModelwithobj ? referenceFor(obj) : nulland using nullish coalescing for destructuring prevents runtime errors when bindings are still loading or undefined. The optional chaining throughout (model?.kind,subjects?.length) maintains consistency.frontend/packages/operator-lifecycle-manager-v1/src/components/cluster-extension/ServiceAccountDropdown.tsx (1)
69-69: Good i18n namespace scoping.Moving the translation key to
olm-v1~namespace is the right call—keeps package-specific strings owned by the package rather than polluting the sharedpublicnamespace.frontend/packages/operator-lifecycle-manager-v1/locales/en/olm-v1.json (1)
51-51: LGTM.The locale key aligns with the
ServiceAccountDropdowncomponent change.frontend/public/locales/en/public.json (1)
703-703: LGTM.Clear error message for template loading failures—good UX.
frontend/public/components/monitoring/alertmanager/alertmanager-config.tsx (1)
601-623: Clean Firehose→hook migration.The
useK8sWatchResourceapproach is the right direction—cleaner component composition, better TypeScript support, and avoids the render-prop pattern. The shape{ data: secret, loaded, loadError }passed toStatusBoxfollows the established console pattern.frontend/packages/helm-plugin/src/components/details-page/resources/__tests__/HelmReleaseResources.spec.tsx (1)
14-36: Good test coverage for the hook migration.Properly validates that:
- The
useK8sWatchResourceshook is invoked (confirming Firehose removal)- Empty state renders correctly when manifest has no resources
The mock shape
[null, true, null]foruseK8sWatchResourcecorrectly represents a loaded state with no data.frontend/public/components/factory/__tests__/list-page.spec.tsx (1)
151-174: Thorough test migration.Good approach—not just checking that the hook was called, but also validating the watch configuration structure. This catches regressions if someone breaks the resource configuration building logic.
frontend/public/components/monitoring/alertmanager/alertmanager-yaml-editor.tsx (1)
130-152: Consistent migration pattern.Mirrors the
alertmanager-config.tsxapproach exactly—good for maintainability. Both Alertmanager views now use the same hook-based resource loading pattern.The type assertion
secret as K8sResourceKindis acceptable given the explicit watch configuration, though in an ideal world the generic type parameter could be specified on the hook call.frontend/public/components/cluster-settings/cluster-settings.tsx (1)
1189-1249: Clean migration from Firehose to useK8sWatchResource.The conditional watch pattern using
nullforhasClusterAutoscaleris the correct approach. ThehorizontalNavPropsconstruction properly aggregates loading states and conditionally includes autoscaler data. This aligns well with the broader PR's hook-driven architecture.frontend/public/components/__tests__/container.spec.tsx (1)
26-34: Test mocks properly updated for hook migration.The mock setup correctly replaces the Firehose mock with
useK8sWatchResource, and tests are appropriately updated to passpodinstead ofobj.data. The strengthened assertion for "Waiting" appearing twice (line 76) improves test reliability.frontend/public/components/instantiate-template.tsx (1)
330-336: Good defensive check for missing template data.This guard catches edge cases where loading completes but data is missing (e.g., watch returns null, or URL params are invalid). The error message is user-friendly and i18n-ready.
frontend/public/components/utils/horizontal-nav.tsx (2)
272-289: Improved explicit prop passing to StatusBox.Moving from spreading
...objto explicit props (data,loaded,loadError) improves clarity and prevents unintended props from leaking intoStatusBox. The optional chaining handles undefinedobjsafely.
32-46: YAML components now explicitly set create={false}.Adding
create={false}to botheditYamlComponentandviewYamlComponentensures the YAML editor is in edit mode rather than create mode. This aligns with the watch-based loading pattern where resources already exist.frontend/public/components/factory/__tests__/details.spec.tsx (1)
60-117: Comprehensive test for extra resources in watch config.The test validates that both the primary
objresource and additionalconfigMapresource are correctly passed touseK8sWatchResources. The shape assertions (lines 93-116) ensure the migration maintains backward compatibility with the expected watch configuration format.
| isLoaded: () => { | ||
| cy.byTestID('skeleton-detail-view').should('not.exist'); | ||
| cy.get('[data-test-id="resource-title"]', { timeout: 30000 }).should('not.be.empty'); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether weak readiness assertions are used elsewhere and whether text-trim checks are standardized.
rg -nP --type=ts -C2 "resource-title.*not\\.be\\.empty|\\.should\\('not\\.be\\.empty'\\)" frontend/packages/integration-tests-cypress
rg -nP --type=ts -C2 "text\\(\\)\\.trim\\(\\)|invoke\\('text'\\)" frontend/packages/integration-tests-cypressRepository: openshift/console
Length of output: 4994
isLoaded() title assertion needs stricter validation to prevent test flakiness
The .should('not.be.empty') check on line 18 can pass with whitespace-only content or before actual text is rendered, risking race conditions in dependent test steps. Enforce visibility and trimmed text content for robust readiness validation.
Suggested fix
isLoaded: () => {
cy.byTestID('skeleton-detail-view').should('not.exist');
- cy.get('[data-test-id="resource-title"]', { timeout: 30000 }).should('not.be.empty');
+ cy.get('[data-test-id="resource-title"]', { timeout: 30000 })
+ .should('be.visible')
+ .should(($title) => {
+ expect($title.text().trim()).to.not.equal('');
+ });
},Aligns with the stricter assertion patterns used elsewhere in the integration test suite (e.g., .invoke('text').should(...)) for meaningful content validation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isLoaded: () => { | |
| cy.byTestID('skeleton-detail-view').should('not.exist'); | |
| cy.get('[data-test-id="resource-title"]', { timeout: 30000 }).should('not.be.empty'); | |
| }, | |
| isLoaded: () => { | |
| cy.byTestID('skeleton-detail-view').should('not.exist'); | |
| cy.get('[data-test-id="resource-title"]', { timeout: 30000 }) | |
| .should('be.visible') | |
| .should(($title) => { | |
| expect($title.text().trim()).to.not.equal(''); | |
| }); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/integration-tests-cypress/views/details-page.ts` around
lines 16 - 19, The isLoaded() readiness check is too weak; after ensuring the
skeleton ('skeleton-detail-view') is gone, update the assertion on
'[data-test-id="resource-title"]' to first assert visibility
(should('be.visible')) and then assert trimmed, non-whitespace content by
invoking the element text and checking it is not empty (e.g., invoke('text') and
assert text.trim() contains non-whitespace or matches /\S/); make this change
inside the isLoaded function to replace the current .should('not.be.empty')
check.
| if (!loaded && !loadError) { | ||
| return ( | ||
| <Firehose resources={resources}> | ||
| <CommandLineTools {...(props as any)} /> | ||
| </Firehose> | ||
| <> | ||
| <DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle> | ||
| <PageHeading title={t('public~Command Line Tools')} /> | ||
| <LoadingBox /> | ||
| </> | ||
| ); | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />; | ||
| }; |
There was a problem hiding this comment.
Missing error state handling.
When loadError is truthy, the component falls through to render CommandLineTools without displaying the error. Consider rendering an error state or StatusBox when loading fails.
🛡️ Proposed fix to handle errors
if (!loaded && !loadError) {
return (
<>
<DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle>
<PageHeading title={t('public~Command Line Tools')} />
<LoadingBox />
</>
);
}
+
+ if (loadError) {
+ return (
+ <>
+ <DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle>
+ <PageHeading title={t('public~Command Line Tools')} />
+ <StatusBox loaded={loaded} loadError={loadError} />
+ </>
+ );
+ }
return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!loaded && !loadError) { | |
| return ( | |
| <Firehose resources={resources}> | |
| <CommandLineTools {...(props as any)} /> | |
| </Firehose> | |
| <> | |
| <DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle> | |
| <PageHeading title={t('public~Command Line Tools')} /> | |
| <LoadingBox /> | |
| </> | |
| ); | |
| }, | |
| ); | |
| } | |
| return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />; | |
| }; | |
| if (!loaded && !loadError) { | |
| return ( | |
| <> | |
| <DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle> | |
| <PageHeading title={t('public~Command Line Tools')} /> | |
| <LoadingBox /> | |
| </> | |
| ); | |
| } | |
| if (loadError) { | |
| return ( | |
| <> | |
| <DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle> | |
| <PageHeading title={t('public~Command Line Tools')} /> | |
| <StatusBox loaded={loaded} loadError={loadError} /> | |
| </> | |
| ); | |
| } | |
| return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/command-line-tools.tsx` around lines 106 - 117,
The component currently only shows LoadingBox when !loaded and !loadError, but
when loadError is truthy it still falls through to render CommandLineTools;
update the conditional to explicitly handle the error case by returning an
error/status UI (e.g., render StatusBox or a dedicated error message) when
loadError is set, ensure you pass the same props or error details to
CommandLineTools if you still want it mounted, and add any required import for
StatusBox; refer to the symbols loaded, loadError, CommandLineTools, LoadingBox,
and StatusBox to locate where to add the new branch.
| obj={{ data: props.pod, loaded: props.loaded, loadError: props.loadError }} | ||
| /> | ||
| <HorizontalNav | ||
| hideNav={true} | ||
| pages={[{ name: 'container', href: '', component: ContainerDetailsList }]} | ||
| obj={props.obj} | ||
| obj={{ data: props.pod, loaded: props.loaded }} | ||
| /> |
There was a problem hiding this comment.
Inconsistent loadError handling between components.
ConnectedPageHeading receives loadError (line 513) but HorizontalNav does not (line 518). Per the horizontal-nav.tsx changes, HorizontalNav now accepts loadError in its type definition. This inconsistency could affect error display behavior in the nav's StatusBox.
🔧 Pass loadError consistently
<HorizontalNav
hideNav={true}
pages={[{ name: 'container', href: '', component: ContainerDetailsList }]}
- obj={{ data: props.pod, loaded: props.loaded }}
+ obj={{ data: props.pod, loaded: props.loaded, loadError: props.loadError }}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| obj={{ data: props.pod, loaded: props.loaded, loadError: props.loadError }} | |
| /> | |
| <HorizontalNav | |
| hideNav={true} | |
| pages={[{ name: 'container', href: '', component: ContainerDetailsList }]} | |
| obj={props.obj} | |
| obj={{ data: props.pod, loaded: props.loaded }} | |
| /> | |
| obj={{ data: props.pod, loaded: props.loaded, loadError: props.loadError }} | |
| /> | |
| <HorizontalNav | |
| hideNav={true} | |
| pages={[{ name: 'container', href: '', component: ContainerDetailsList }]} | |
| obj={{ data: props.pod, loaded: props.loaded, loadError: props.loadError }} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/container.tsx` around lines 513 - 519,
ConnectedPageHeading is being passed loadError but HorizontalNav is not; update
the HorizontalNav invocation to include the same loadError so its StatusBox can
render errors consistently. Specifically, in the component where
ConnectedPageHeading and HorizontalNav are rendered, add loadError:
props.loadError to the obj prop passed to HorizontalNav (the HorizontalNav call
that currently passes obj={{ data: props.pod, loaded: props.loaded }}), ensuring
ContainerDetailsList, props.pod, props.loaded and props.loadError are all
forwarded consistently.
| if (!loaded && !loadError) { | ||
| return <LoadingBox />; | ||
| } | ||
|
|
||
| if (loadError) { | ||
| return <ErrorPage404 />; | ||
| } |
There was a problem hiding this comment.
Error handling may obscure actual error type.
Rendering ErrorPage404 for all loadError cases is misleading. A 403 Forbidden or network timeout would show "Page Not Found" instead of an appropriate error message. Consider checking the error status code or using a generic error component that displays the actual error.
🛠️ Suggested approach
- if (loadError) {
- return <ErrorPage404 />;
+ if (loadError) {
+ // Check if it's actually a 404, otherwise show the real error
+ if (loadError?.response?.status === 404) {
+ return <ErrorPage404 />;
+ }
+ return <LoadError label={props.kind}>{loadError.message || 'Failed to load resource'}</LoadError>;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/create-yaml.tsx` around lines 128 - 134, The
current error handling in the create-yaml component returns ErrorPage404 for any
loadError, which misrepresents errors like 403 or network failures; update the
render logic around the loaded and loadError checks (variables loaded,
loadError) to inspect loadError (e.g., status or error.type/message) and render
an appropriate component instead of always ErrorPage404—either map specific
statuses to dedicated components (e.g., ForbiddenPage for 403) or introduce a
GenericError component that displays the actual error.message/status so users
see the real failure instead of a 404.
| // TODO: Remove the resources prop and the fallback ?? resources after all components are migrated from Firehose to hooks. | ||
| // Use watchedResources (from useK8sWatchResources) if available, fallback to resources (from Firehose) | ||
| const resourceData = watchedResources ?? resources; | ||
| const data = flatten ? flatten(resourceData) : []; |
There was a problem hiding this comment.
Don't switch to watchedResources until it has entries.
useK8sWatchResources can be {} on the first render, so watchedResources ?? resources prefers the empty object and still calls flatten. Existing callers that do resources.foo.data will throw, and the Firehose fallback in the TODO never activates.
🔧 Suggested fix
- const resourceData = watchedResources ?? resources;
- const data = flatten ? flatten(resourceData) : [];
+ const resourceData =
+ watchedResources && !_.isEmpty(watchedResources) ? watchedResources : resources;
+ const data = flatten && resourceData ? flatten(resourceData) : [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: Remove the resources prop and the fallback ?? resources after all components are migrated from Firehose to hooks. | |
| // Use watchedResources (from useK8sWatchResources) if available, fallback to resources (from Firehose) | |
| const resourceData = watchedResources ?? resources; | |
| const data = flatten ? flatten(resourceData) : []; | |
| // TODO: Remove the resources prop and the fallback ?? resources after all components are migrated from Firehose to hooks. | |
| // Use watchedResources (from useK8sWatchResources) if available, fallback to resources (from Firehose) | |
| const resourceData = | |
| watchedResources && !_.isEmpty(watchedResources) ? watchedResources : resources; | |
| const data = flatten && resourceData ? flatten(resourceData) : []; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/factory/list-page.tsx` around lines 108 - 111, The
current selection of resourceData uses watchedResources ?? resources which picks
an empty object from useK8sWatchResources on first render and breaks callers;
update the logic in the list-page component to only use watchedResources when it
actually has entries (e.g. check watchedResources is truthy and
Object.keys(watchedResources).length > 0) otherwise fall back to resources, and
keep the subsequent use of flatten(resourceData) unchanged (references:
watchedResources, resources, resourceData, flatten, useK8sWatchResources).
| reduxIDs: propsReduxIDs, | ||
| filterList: propsFilterList, |
There was a problem hiding this comment.
Dispatch the filter action here.
Line 617 passes the raw filterList action creator into FireMan, but applyFilterInternal just invokes it and drops the return value. URL-applied filters and any applyFilter consumer will no-op.
🔧 Suggested fix
export const FireMan: FC<FireManProps & { filterList?: typeof filterList }> = (props) => {
const {
resources,
textFilter,
canCreate,
@@
- filterList: propsFilterList,
+ filterList: propsFilterList = filterList,
} = props;
+ const dispatch = useConsoleDispatch();
const navigate = useNavigate();
@@
const applyFilterInternal = useCallback(
(filterName: string, options: any) => {
if (['q', 'kind', 'orderBy', 'sortBy'].includes(filterName)) {
return;
}
if (filterName.indexOf(storagePrefix) === 0) {
return;
}
- reduxIDs.forEach((id) => propsFilterList(id, filterName, options));
+ reduxIDs.forEach((id) => dispatch(propsFilterList(id, filterName, options)));
},
- [reduxIDs, propsFilterList],
+ [dispatch, reduxIDs, propsFilterList],
);Also applies to: 202-218, 617-617
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/factory/list-page.tsx` around lines 176 - 177, The
bug is that FireMan is being given the raw action creator filterList
(propsFilterList) but applyFilterInternal calls it and ignores the returned
action, so dispatch never occurs; update the place where FireMan is constructed
(and any other occurrences around lines with reduxIDs/ filterList and where
applyFilterInternal is used) to pass a bound dispatcher function instead of the
raw creator—i.e., wrap propsFilterList with dispatch (or call
props.dispatch(propsFilterList(...))) so applyFilter / applyFilterInternal
receive and return a dispatched action; ensure functions named FireMan,
applyFilterInternal, filterList (propsFilterList) and applyFilter are updated
accordingly so URL-applied filters and applyFilter consumers receive the
dispatched result.
| // Build resources configuration for FireMan (needs prop for redux IDs) | ||
| const k8sResources = useMemo( | ||
| () => | ||
| _.map(props.resources, (r) => ({ | ||
| ...r, | ||
| isList: r.isList !== undefined ? r.isList : true, | ||
| namespace: r.namespaced ? namespace : r.namespace, | ||
| prop: r.prop || r.kind, | ||
| })), | ||
| [props.resources, namespace], | ||
| ); | ||
|
|
||
| // Build watch resources configuration for useK8sWatchResources | ||
| const watchResources = useMemo(() => { | ||
| if (mock) { | ||
| return {}; | ||
| } | ||
| return k8sResources.reduce((acc, r) => { | ||
| const key = r.prop || r.kind; | ||
| acc[key] = { | ||
| kind: r.kind, | ||
| name: r.name, | ||
| namespace: r.namespace, | ||
| isList: r.isList, | ||
| selector: r.selector, | ||
| fieldSelector: r.fieldSelector, | ||
| limit: r.limit, | ||
| namespaced: r.namespaced, | ||
| optional: r.optional, | ||
| }; | ||
| return acc; | ||
| }, {} as Record<string, WatchK8sResource>); | ||
| }, [k8sResources, mock]); |
There was a problem hiding this comment.
Preserve resources[].filters in the watch path.
Line 404 still threads filters into each resource config, and Line 415 assumes the list content is already narrowed when that prop is used. This conversion never applies r.filters to watchedResources, so pages relying on ListPageProps.filters will now render the full dataset and incorrect row-filter counts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/factory/list-page.tsx` around lines 533 - 565, The
watchResources builder omits resource-level filters so ListPageProps.filters are
not applied to useK8sWatchResources; update the reduce that creates
watchResources (based on k8sResources) to copy r.filters into each watch config
(e.g., include filters: r.filters) so the produced record passed to
useK8sWatchResources preserves resources[].filters and downstream list rendering
and row counts are filtered correctly; modify the reduce in the watchResources
useMemo that constructs acc[key] to include the filters property from r.
4dc27c6 to
5e342b6
Compare
|
@rhamilto: This pull request references CONSOLE-5091 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@rhamilto: This pull request references CONSOLE-5091 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
needs-rebase
|
@rhamilto: This pull request references CONSOLE-5091 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
removed
the
needs-rebase
|
@logonoff I had to add a type cast fix in commit 5e00ad7 to resolve a DropEvent type conflict between react-dropzone and PatternFly that appeared after updating to @patternfly/react-component-groups@6.4.0-prerelease.15. The cast is at droppable-edit-yaml.tsx:144. |
Adds bulk selection support and schedulable/unschedulable bulk actions to the Nodes list page using PatternFly DataView selection patterns. Changes: - Add selection support to ConsoleDataView with optional onSelectAll callback - Add useDataViewSelection hook for managing row selection state - Add useBulkNodeActions hook with mark schedulable/unschedulable actions - Add actionsBreakpoint prop to ConsoleDataView (default: 'lg') - Update @patternfly/react-component-groups to 6.4.0-prerelease.15 - Fix DropEvent type conflict in droppable-edit-yaml - Remove isActionCell prop from actionsCellProps to fix React DOM warning Note: The resolution for react-component-groups uses npm registry URL to bypass the npmMinimalAgeGate (3 day) security check. This can be removed after 2026-03-29 when the package is 3 days old. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@rhamilto: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@rhamilto: This pull request references CONSOLE-5091 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
7 participants
Summary
Implements row selection with checkboxes for the Nodes list page, allowing users to select multiple nodes and perform bulk actions to mark them as schedulable or unschedulable.
Demo
Screen.Recording.2026-03-26.at.11.47.37.AM.mov
Screen.Recording.2026-03-26.at.2.49.01.PM.mov
Changes
useDataViewSelection,dataViewSelectionHelpersactionsBreakpointprop for responsive toolbar layout (default: 'lg')OverflowMenuProps['breakpoint']Features
Promise.allSettled()with granular feedback for partial failuresPackage Updates
This PR uses
@patternfly/react-component-groups@6.4.0-prerelease.15.The package is installed via direct npm registry URL in
resolutionsto bypass thenpmMinimalAgeGate(3 day) security check:Why this version: The prerelease includes a fix for ResponsiveActions that prevents empty dropdowns when all actions are persistent/pinned.
Why the npm URL: The package was published on 2026-03-26. The console repo has
npmMinimalAgeGate: 3din.yarnrc.ymlwhich blocks packages less than 3 days old as a security measure. The npm URL bypasses this check.Cleanup: After 2026-03-29 (3 days after publication), update to use standard version number or wait for stable release.
Known issue: Console warning about ResponsiveAction accessing
keyprop - tracked in patternfly/react-component-groups#890Dependencies
Test plan
🤖 Generated with Claude Code